Skip to content

Conversation

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 15, 2021

This PR builds on and currently includes PR #727, thus the draft status.

This removes Travis CI config and adds GHA CI. You can see how it looks like at https://github.com/kolyshkin/runtime-tools/actions/runs/1361288633 and kolyshkin#1

Also:

  • fix many linter warnings from the default golangci-lint config
    (alas not all of them from errcheck -- there are way too many 😿, I gave up)
  • add golangci-lint, fix all its warnings
  • re-add commit subject length validation

Addresses: #726

@kolyshkin kolyshkin force-pushed the gha-ci branch 2 times, most recently from 1ec79b2 to 4857041 Compare October 15, 2021 02:34
Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks sane to me -- do you think it would be useful for me to try pushing your branch to a new branch here on the origin so the PR can actually run the actions?

(I don't personally have a strong preference, but other folks might.)

@kolyshkin
Copy link
Contributor Author

This all looks sane to me -- do you think it would be useful for me to try pushing your branch to a new branch here on the origin so the PR can actually run the actions?

As noted in description, I have those ran at https://github.com/kolyshkin/runtime-tools/actions/runs/1344345504, don't think it makes any difference which repo it is.

This is still WIP; currently working on enabling golangci-lint and it's overwhelming :( Guess I'll have to disable errcheck linter :(

I'll let you know once it's ready @tianon, thanks for looking!

@kolyshkin kolyshkin changed the title Switch to GHA CI [WIP] Switch to GHA CI Oct 15, 2021
@tianon
Copy link
Member

tianon commented Oct 15, 2021 via email

@kolyshkin kolyshkin changed the title [WIP] Switch to GHA CI Switch to GHA CI Oct 15, 2021
@kolyshkin
Copy link
Contributor Author

This PR builds on and currently includes PR #727, thus the draft status. Will rebase once #727 is merged.

@kolyshkin
Copy link
Contributor Author

@kolyshkin
Copy link
Contributor Author

Can we merge #727 so we can merge this one, and (re)enable CI for this repo? I have more changes that are piling up but this one is a blocker as we can't review PRs without CI.

@tianon
Copy link
Member

tianon commented Oct 19, 2021

Thanks to Travis it's going to require a repository administrator (which excludes me 😅).

@vbatts is that you? @caniszczyk?

IMO both PRs look great and merging this which contains both seems like a simpler path, but I don't have any issue with any solution that unblocks Kir and gets this repo able to progress again. 😁

@vbatts
Copy link
Member

vbatts commented Oct 19, 2021 via email

@caniszczyk
Copy link
Contributor

GHA shouldn't require anything on my end, IMHO let's drop TravisCI fast

@caniszczyk
Copy link
Contributor

caniszczyk commented Oct 19, 2021

I truly don't know how to disable the Travis CI check

I pulled the webhook and couldn't find any other integrations

@kolyshkin
Copy link
Contributor Author

@caniszczyk this is not about disabling Travis, this is about removing travis (and, maybe, pullapprove, too) from the list of jobs required to merge a PR. This is under Settings -> Branches -> Branch protection rules -> master -> edit -> Status checks that are required. Need to remove travis-ci from that list (and later add GHA CI jobs added by this PR).

@caniszczyk
Copy link
Contributor

caniszczyk commented Oct 19, 2021 via email

1. Remove .gofmt and .golint targets -- they are to be replaced
   by golangci-lint a bit later.

2. Simplify .gotest target, add support for TESTFLAGS

Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
1. Move the common go build flags to BUILD_FLAGS. Add an ability to
   specify more build flags via EXTRA_FLAGS.

2. Add STATIC_BUILD_FLAGS, use it for runtimetest static build.
   Commit 95617cb added CGO_ENABLED=0 in order to produce a static
   build, which was the way things worked back in the day. Now we can
   use netgo, and we have to specify "-extldflags -static" in -ldflags
   in order to have a static build.

3. Enable to build binaries with -race from CI.

Signed-off-by: Kir Kolyshkin <[email protected]>
Now, the commit ID will contain the closest tag, the number of commits
since the tag, and the (abbreviated) git commit sha (see example below).

This tag is still unique and can be used instead of bare sha  for all
git commands.

Before:
	$ ./runtimetest -version
	runtimetest version 0.9.0, commit: ea2948e

After:
	$ ./runtimetest -version
	runtimetest version 0.9.0, commit: v0.9.0-28-ge91e387

This means that
 - the closest tag is v0.9.0
 - there were 28 commits after the tag
 - the abbreviated sha is e91e387

Nice, isn't it?

While at it,
 - allow to set commit id via environment (might be needed for distros);
 - remove unneeded "|| true" stance.

Signed-off-by: Kir Kolyshkin <[email protected]>
Fix the following warnings by removing the unused stuff.

> generate/seccomp/consts.go:7:2: `kill` is unused (deadcode)
> 	kill             = "kill"
> 	^
> generate/seccomp/consts.go:8:2: `trap` is unused (deadcode)
> 	trap             = "trap"
> 	^
> generate/seccomp/consts.go:9:2: `trace` is unused (deadcode)
> 	trace            = "trace"
> 	^
> generate/seccomp/consts.go:10:2: `allow` is unused (deadcode)
> 	allow            = "allow"
> 	^
> generate/seccomp/consts.go:11:2: `errno` is unused (deadcode)
> 	errno            = "errno"
> 	^
> generate/seccomp/syscall_compare.go:95:6: `identicalExceptAction` is unused (deadcode)
> func identicalExceptAction(config1, config2 *rspec.LinuxSyscall) bool {
>      ^
> generate/seccomp/syscall_compare.go:103:6: `identicalExceptArgs` is unused (deadcode)
> func identicalExceptArgs(config1, config2 *rspec.LinuxSyscall) bool {
>      ^

All of the above was added by commit 60473d2 but never ever used.

> generate/generate.go:1567:6: `strPtr` is unused (deadcode)
> func strPtr(s string) *string { return &s }
>      ^

This was added by commit 80e63b3, but later in afc8d35
all its uses were removed.

Signed-off-by: Kir Kolyshkin <[email protected]>
These ones:

generate/generate.go:1563:2: S1023: redundant `return` statement (gosimple)
	return
	^
validation/util/linux_resources_devices.go:29:6: S1002: should omit comparison to bool constant, can be simplified to `device.Allow` (gosimple)
		if device.Allow == true {
		   ^
cmd/runtimetest/main.go:985:94: S1039: unnecessary use of fmt.Sprintf (gosimple)
		rfcError, err := c.Ok(actual == expected, specerror.LinuxProcOomScoreAdjSet, spec.Version, fmt.Sprintf("has expected OOM score adjustment"))
		                                                                                           ^
validation/hostname/hostname.go:34:13: S1039: unnecessary use of fmt.Sprintf (gosimple)
		t.Skip(1, fmt.Sprintf("linux-specific namespace test"))
		          ^
validation/linux_ns_itype/linux_ns_itype.go:122:13: S1039: unnecessary use of fmt.Sprintf (gosimple)
		t.Skip(1, fmt.Sprintf("linux-specific namespace test"))
		          ^

Signed-off-by: Kir Kolyshkin <[email protected]>
Fix deprecation warnings related to commit 84a62c6 from 2016. In
particular, these ones:

validation/util/test.go:299:15: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
		if err := f(g.Spec(), t, &state); err != nil {
		            ^
validation/config_updates_without_affect/config_updates_without_affect.go:71:2: SA1019: g.SetSpec is deprecated: Replace with: (staticcheck)
	g.SetSpec(spec)
	^
validation/delete_resources/delete_resources.go:62:44: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
	if err := util.ValidateLinuxResourcesPids(g.Spec(), t, &state); err != nil {
	                                          ^
validation/hooks/hooks.go:30:40: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
			output = filepath.Join(r.BundleDir, g.Spec().Root.Path, "output")
			                                    ^
validation/killsig/killsig.go:37:38: SA1019: sigConfig.Spec is deprecated: Replace with generator.Config. (staticcheck)
	rootDir := filepath.Join(bundleDir, sigConfig.Spec().Root.Path)
	                                    ^
validation/misc_props/misc_props.go:65:24: SA1019: annotationConfig.Spec is deprecated: Replace with generator.Config. (staticcheck)
		{extendedSpec{Spec: *annotationConfig.Spec()}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, true, specerror.NewError(specerror.AnnotationsKeyIgnoreUnknown, fmt.Errorf("implementations that are reading/processing this configuration file MUST NOT generate an error if they encounter an unknown annotation key"), rspecs.Version)},
		                     ^
validation/misc_props/misc_props.go:66:24: SA1019: basicConfig.Spec is deprecated: Replace with generator.Config. (staticcheck)
		{extendedSpec{Spec: *basicConfig.Spec(), Unknown: "unknown"}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, true, specerror.NewError(specerror.ExtensibilityIgnoreUnknownProp, fmt.Errorf("runtimes that are reading or processing this configuration file MUST NOT generate an error if they encounter an unknown property"), rspecs.Version)},
		                     ^
validation/misc_props/misc_props.go:67:24: SA1019: invalidConfig.Spec is deprecated: Replace with generator.Config. (staticcheck)
		{extendedSpec{Spec: *invalidConfig.Spec()}, util.LifecycleActionCreate | util.LifecycleActionStart | util.LifecycleActionDelete, false, specerror.NewError(specerror.ValidValues, fmt.Errorf("runtimes that are reading or processing this configuration file MUST generate an error when invalid or unsupported values are encountered"), rspecs.Version)},
		                     ^
validation/prestart/prestart.go:31:40: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
			output = filepath.Join(r.BundleDir, g.Spec().Root.Path, "output")
			                                    ^
validation/prestart/prestart.go:33:38: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
				Path: filepath.Join(r.BundleDir, g.Spec().Root.Path, "/bin/sh"),
				                                 ^
validation/prestart_fail/prestart_fail.go:30:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
		Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/false"),
		                               ^
validation/poststop_fail/poststop_fail.go:31:37: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
	output := filepath.Join(bundleDir, g.Spec().Root.Path, "output")
	                                   ^
validation/poststop_fail/poststop_fail.go:33:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
		Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/false"),
		                               ^
validation/poststop_fail/poststop_fail.go:38:34: SA1019: g.Spec is deprecated: Replace with generator.Config. (staticcheck)
		Path: filepath.Join(bundleDir, g.Spec().Root.Path, "/bin/sh"),
		                               ^

...and so on (too many to list).

Signed-off-by: Kir Kolyshkin <[email protected]>
This code was added by commits 9b1de8c, fc0fc84, and f5e59a3.
Since these new struct members are not pointers, no initialization is
needed. Remove the initializers, call the parent initializers from the
users.

This fixes the following warnings:

generate/config.go:191:5: SA4022: the address of a variable cannot be nil (staticcheck)
	if &g.Config.VM.Hypervisor == nil {
	   ^
generate/config.go:198:5: SA4022: the address of a variable cannot be nil (staticcheck)
	if &g.Config.VM.Kernel == nil {
	   ^
generate/config.go:205:5: SA4022: the address of a variable cannot be nil (staticcheck)
	if &g.Config.VM.Image == nil {
	   ^

Signed-off-by: Kir Kolyshkin <[email protected]>
Commit 20a71e4 implemented a number of improvements, including a
modification to validatePosixMounts, which results in the following
linter warning:

> cmd/runtimetest/main.go:1179:4: SA4006: this value of `rfcError` is never used (staticcheck)
> 			rfcError, err = c.Ok(false, specerror.MountsInOrder, spec.Version, fmt.Sprintf("mounts[%d] (%s) found", i, configMount.Destination))
>			^

It seems that c.harness.YAML() call was mistakenly placed into the
"else" code block. Move it out.

Signed-off-by: Kir Kolyshkin <[email protected]>
Fix the following warning:

> validate/validate.go:134:2: SA4006: this value of `err` is never used (staticcheck)
> 	configRenamedToConfigSchemaVersion, err := semver.Parse("1.0.0-rc2") // config.json became config-schema.json in 1.0.0-rc2
>	^

As the error is unexpected here, ignore it.

Signed-off-by: Kir Kolyshkin <[email protected]>
Motivated by this linter warning:

> validation/util/container.go:116:3: SA4006: this value of `err` is never used (staticcheck)
> 		err = err2
> 		^

Rather than fixing it, let's use bytes.Buffer (rather than files) for
stdout and stderr, simplifying the whole thing.

Signed-off-by: Kir Kolyshkin <[email protected]>
It is only called from defer, so it does not make sense to return an
error. So, log errors to stderr instead of returning them.

Also, log errors from Kill and WaitingForStatus.

Signed-off-by: Kir Kolyshkin <[email protected]>
We do not expect any errors from YAML function, so explicitly ignore
those to silence the errcheck linter.

Signed-off-by: Kir Kolyshkin <[email protected]>
These functions never return errors, so let's simplify them and their
callers.

This fixes a bunch of errcheck linter warning, as some callers of these
methods were not checking for errors.

Signed-off-by: Kir Kolyshkin <[email protected]>
Had to disable errcheck linter, as there are too many warnings
yet to be fixed.

Signed-off-by: Kir Kolyshkin <[email protected]>
To replace the one removed by commit 5432bc4.

Signed-off-by: Kir Kolyshkin <[email protected]>
@kolyshkin kolyshkin marked this pull request as ready for review October 19, 2021 23:10
func (r *Runtime) Clean(removeBundle bool, forceRemoveBundle bool) error {
r.Kill("KILL")
WaitingForStatus(*r, LifecycleStatusStopped, time.Second*10, time.Second/10)
func (r *Runtime) Clean(removeBundle bool, forceRemoveBundle bool) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not familiar enough with this codebase.. is this a API-breaking change? Does it matter? (comment applies to other spots too)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was literally about to merge.
Should I wait?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vbatts - if @kolyshkin made the change, I am assuming nothing to worry about. Plus this repo is pre-1.0. Press that button!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a copy/paste from the relevant commit message:

validation: fix Clean

It is only called from defer, so it does not make sense to return an
error. So, log errors to stderr instead of returning them.

Also, log errors from Kill and WaitingForStatus.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API concern is valid though, but apparently validation/util is de-facto internal package, only used by runtime-tools (checked via https://sourcegraph.com/search?q=context:global+opencontainers/runtime-tools/validation/util&patternType=literal)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-checked with pkg.go.dev -- same thing, it is only imported internally (and by forks) https://pkg.go.dev/github.com/opencontainers/runtime-tools/validation/util?tab=importedby

Ideally, such packages should be moved to under internal/ but it's out of scope for this PR (which is already big enough).

Copy link

@jdolitsky jdolitsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@vbatts vbatts merged commit 17ac4c5 into opencontainers:master Oct 20, 2021
@caniszczyk
Copy link
Contributor

NICE, great work all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants